Repository navigation
Fetch completed async results without an extra status request - #1056
aakash-saravanan-db wants to merge 2 commits into
Conversation
Signed-off-by: Aakash Saravanan <aakash.saravanan@databricks.com>
GitHub Actions
Failure
GitHub Actions
Inactive
![peco-review-bot[bot]](https://avatars.githubusercontent.com/in/3801177?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: 1 Medium · 2 Low
Small, mostly mechanical change with one medium concern: the switch from await_result() to fetch_result() is not tied to the dependency floor or checked by any test that would notice a missing method, because all handles are permissive MagicMocks. The cursor-side SUCCEEDED check that makes skipping the extra status request safe does exist.
| else self._kernel_session.attach_async_statement(command_id.guid) | ||
| ) | ||
| stream = handle.await_result() | ||
| # Cursor.get_async_execution_result has already confirmed success. |
There was a problem hiding this comment.
🔵 Low — Correctness now depends on a precondition that lives only in a comment. get_execution_result assumes the caller has already seen SUCCEEDED, and that only holds because Cursor.get_async_execution_result happens to poll first. await_result() was safe for any caller. fetch_result() on a statement that is still pending or has failed may return a partial or empty stream, or a confusing error, depending on kernel behavior. Please state the precondition in the get_execution_result docstring, or add a unit test that pins the cursor-side ordering (poll → SUCCEEDED → fetch), so a future caller that skips the poll is caught.
| c = _make_client() | ||
| c._kernel_session = MagicMock() | ||
| owner = MagicMock(spec=["status", "await_result"]) | ||
| owner = MagicMock(spec=["status", "fetch_result"]) |
There was a problem hiding this comment.
🔵 Low — The rename stopped short of kernel/result_set.py. Its module docstring (line 9) still says ResultStream comes from ExecutedAsyncStatement.await_result(), which no longer matches the async-result code path. Please update it to fetch_result() to keep the docs consistent with the code.
Signed-off-by: Aakash Saravanan <aakash.saravanan@databricks.com>
GitHub Actions
Failure
GitHub Actions
Inactive
GitHub Actions
Inactive
There was a problem hiding this comment.
Verdict: 1 Low
Looks good, with one low-severity concern: the tests use permissive mocks, so they cannot show that fetch_result() exists on every handle type it is now called on. The cursor only calls get_execution_result after get_query_state() reports SUCCEEDED, so skipping the second status poll is safe, and the lock, retry and stream-started-marker handling is unchanged.
GitHub Actions
Error
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
There was a problem hiding this comment.
Verdict: 1 Low
The change looks right, with one low-severity concern I couldn't verify: whether the pinned kernel wheel (^1.4.0) actually provides fetch_result(). I confirmed that Cursor.get_async_execution_result checks for CommandState.SUCCEEDED before it calls get_execution_result, and the swap leaves the concurrency guard, retry handling and marker handling unchanged.
| ) | ||
| stream = handle.await_result() | ||
| # Cursor.get_async_execution_result has already confirmed success. | ||
| stream = handle.fetch_result() |
There was a problem hiding this comment.
🔵 Low — This is the first place the connector calls ExecutedAsyncStatement.fetch_result() (and reader().fetch_result() on the re-fetch path). Before this change, the only result call in src/databricks/sql/backend/kernel/ was await_result(). The dependency floor is still databricks-sql-kernel ^1.4.0 (pyproject.toml:73). I couldn't check the kernel source from here, so I can't confirm that 1.4.0 ships fetch_result on both the owning handle and the derived reader. If it doesn't, every get_async_execution_result() on use_kernel=True fails with AttributeError, which the generic except Exception turns into a mapped DB error. That would break a path that works today.
The unit tests can't catch this. They use plain MagicMock() handles, which accept any attribute name, so they would pass whether or not the method exists.
Please confirm fetch_result is in the 1.4.0 wheel's ExecutedAsyncStatement and reader surface. If it isn't, bump the floor to the first version that has it. A spec=[...]-restricted handle mock in one of these tests would also lock in the method name.
|
Superseded for the coordinated native release by terminal-response reuse in kernel #451, which is merged upstream and included in the Volume native candidate. With that rebuilt native, traced public-cursor tests show identical request counts and rows/metadata for Closing this PR without merge; its branch and validation artifacts remain preserved. Compatible native release/package activation remains a separate prerequisite. |
Feature or gap
Retrieving an already completed asynchronous result sends another status request on the kernel backend.
What this PR addresses
Use the existing direct result-fetch method after the cursor confirms completion, retaining concurrent-reader, retry and error handling.
Dependencies
None; this uses the existing native result-fetch API.
Tests and validation
All 268 affected Python client/result-set tests pass on this standalone change. Independent matched live async measurements meet the ±10% parity target; PUT +4.41%, GET +9.48%, REMOVE +4.37%, lifecycle +5.24% in the combined connector.